Skip to content

Conversation

@agnetemoos
Copy link

@agnetemoos agnetemoos commented Sep 18, 2025

Link to ticket

Fixes #309

Description

Add BRND Booking feed type

@turegjorup turegjorup requested a review from tuj September 25, 2025 08:14
@turegjorup turegjorup requested review from turegjorup and removed request for tuj October 10, 2025 12:59
Copy link
Contributor

@tuj tuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, looks good.
There are a few issues I think you should handle.

],
'api_auth_key' => [
'type' => 'string',
'exposeValue' => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should hide this value, otherwise everybody in the admin can see the value.

Suggested change
'exposeValue' => true,
'exposeValue' => false,

It will still be possible to set the value, it will just not be visible in the admin after being set.


$bookings = $this->apiClient->getInfomonitorBookingsDetails($feedSource, $sportCenterId);

$result['bookings'] = array_map([$this, 'parseBrndBooking'], $bookings);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to filter out entries that do not meet some minimum requirements (startTime, endTime, activity, ?)
Otherwise, you can end up with slides that display broken data.

I would replace array_map with array_reduce and add some validation checks and ignore broken entries.

$startDateTime = null;
if (!empty($booking['dato']) && isset($booking['starttid']) && is_string($booking['starttid'])) {
// Trim starttid to 6 digits after dot for microseconds
$starttid = preg_replace('/\.(\d{6})\d+$/', '.$1', $booking['starttid']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace danish variable name with english.
Maybe: $startTimeString

// Parse end time
$endDateTime = null;
if (!empty($booking['dato']) && isset($booking['sluttid']) && is_string($booking['sluttid'])) {
$sluttid = preg_replace('/\.(\d{6})\d+$/', '.$1', $booking['sluttid']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace danish variable name with english.
Maybe: $endTimeString

$starttid = preg_replace('/\.(\d{6})\d+$/', '.$1', $booking['starttid']);
$dateOnly = substr($booking['dato'], 0, 10);
$dateTimeString = $dateOnly.' '.$starttid;
$startDateTime = \DateTimeImmutable::createFromFormat('m/d/Y H:i:s.u', $dateTimeString);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this throws an exception or returns false?
https://www.php.net/manual/en/datetime.createfromformat.php

$sluttid = preg_replace('/\.(\d{6})\d+$/', '.$1', $booking['sluttid']);
$dateOnly = substr($booking['dato'], 0, 10);
$dateTimeString = $dateOnly.' '.$sluttid;
$endDateTime = \DateTimeImmutable::createFromFormat('m/d/Y H:i:s.u', $dateTimeString);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this throws an exception or returns false?
https://www.php.net/manual/en/datetime.createfromformat.php

];
}

public function getData(Feed $feed): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure this function does not throw an exception.
It should always return $result, so the client does not have to handle the exceptions.

@tuj tuj added the enhancement New feature or request label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BRND dataintegration

2 participants